Skip to content

Bind RHS of comma expressions too#47049

Merged
jakebailey merged 5 commits intomicrosoft:mainfrom
jakebailey:fix-41312
Jan 18, 2022
Merged

Bind RHS of comma expressions too#47049
jakebailey merged 5 commits intomicrosoft:mainfrom
jakebailey:fix-41312

Conversation

@jakebailey
Copy link
Member

@jakebailey jakebailey commented Dec 7, 2021

Fixes #44487

This is an extension of #41312, applying maybeBindExpressionFlowIfCall the RHS too. The argument there was:

just like lists of statements, they generally only exist to compose lists of possibly-side-effecting actions (like, say, assertions).

I'd argue that this means that the entire chain of items in a comma expression (of any size) should have this applied, since they are being thought of here as a list of statements with potential side effects, and not just the leftmost expression.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Dec 7, 2021
@jakebailey
Copy link
Member Author

jakebailey commented Dec 7, 2021

@typescript-bot perf test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 7, 2021 •
edited
Loading

Heya @jakebailey, I've started to run the parallelized community code test suite on this PR at 5be2594. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 7, 2021 •
edited
Loading

Heya @jakebailey, I've started to run the perf test suite on this PR at 5be2594. You can monitor the build here.

Update: The results are in!

@typescript-bot typescript-bot mentioned this pull request Dec 7, 2021
Open
@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 7, 2021

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 7, 2021

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..47049

Metric main 47049 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 354,951k (± 0.02%) 354,924k (± 0.01%) -27k (- 0.01%) 354,754k 355,037k
Parse Time 1.95s (± 0.34%) 1.95s (± 0.43%) +0.00s (+ 0.10%) 1.93s 1.97s
Bind Time 0.85s (± 1.11%) 0.85s (± 0.56%) -0.00s (- 0.35%) 0.84s 0.86s
Check Time 5.54s (± 0.43%) 5.50s (± 0.46%) -0.03s (- 0.63%) 5.45s 5.56s
Emit Time 5.90s (± 0.46%) 5.88s (± 0.56%) -0.02s (- 0.32%) 5.81s 5.94s
Total Time 14.23s (± 0.38%) 14.18s (± 0.42%) -0.05s (- 0.34%) 14.03s 14.29s
Compiler-Unions - node (v10.16.3, x64)
Memory used 204,133k (± 0.03%) 204,108k (± 0.04%) -25k (- 0.01%) 203,979k 204,278k
Parse Time 0.79s (± 0.73%) 0.79s (± 0.85%) -0.00s (- 0.38%) 0.77s 0.80s
Bind Time 0.52s (± 1.00%) 0.52s (± 1.44%) -0.00s (- 0.39%) 0.50s 0.53s
Check Time 7.88s (± 0.41%) 7.87s (± 0.42%) -0.01s (- 0.14%) 7.80s 7.94s
Emit Time 2.48s (± 0.98%) 2.46s (± 0.62%) -0.02s (- 0.81%) 2.44s 2.49s
Total Time 11.67s (± 0.39%) 11.64s (± 0.33%) -0.03s (- 0.27%) 11.55s 11.73s
Monaco - node (v10.16.3, x64)
Memory used 342,477k (± 0.02%) 342,407k (± 0.02%) -70k (- 0.02%) 342,207k 342,571k
Parse Time 1.47s (± 0.75%) 1.49s (± 0.75%) +0.01s (+ 0.88%) 1.46s 1.51s
Bind Time 0.74s (± 0.67%) 0.75s (± 0.87%) +0.00s (+ 0.40%) 0.73s 0.76s
Check Time 5.53s (± 0.48%) 5.50s (± 0.58%) -0.03s (- 0.56%) 5.43s 5.58s
Emit Time 3.21s (± 1.43%) 3.23s (± 1.25%) +0.02s (+ 0.65%) 3.14s 3.32s
Total Time 10.96s (± 0.53%) 10.96s (± 0.61%) +0.00s (+ 0.04%) 10.88s 11.12s
TFS - node (v10.16.3, x64)
Memory used 305,554k (± 0.01%) 305,606k (± 0.03%) +52k (+ 0.02%) 305,480k 305,947k
Parse Time 1.20s (± 0.54%) 1.20s (± 0.54%) 0.00s ( 0.00%) 1.18s 1.21s
Bind Time 0.71s (± 0.66%) 0.71s (± 0.99%) -0.00s (- 0.14%) 0.70s 0.73s
Check Time 5.05s (± 0.35%) 5.06s (± 0.47%) +0.01s (+ 0.20%) 5.00s 5.11s
Emit Time 3.39s (± 1.52%) 3.39s (± 2.19%) -0.01s (- 0.21%) 3.26s 3.63s
Total Time 10.35s (± 0.49%) 10.35s (± 0.82%) -0.00s (- 0.01%) 10.19s 10.62s
material-ui - node (v10.16.3, x64)
Memory used 471,361k (± 0.01%) 471,358k (± 0.02%) -3k (- 0.00%) 471,148k 471,583k
Parse Time 1.77s (± 0.42%) 1.78s (± 0.38%) +0.01s (+ 0.34%) 1.76s 1.79s
Bind Time 0.66s (± 0.68%) 0.66s (± 0.61%) +0.00s (+ 0.46%) 0.65s 0.67s
Check Time 14.25s (± 0.46%) 14.28s (± 0.64%) +0.03s (+ 0.21%) 14.11s 14.58s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.67s (± 0.44%) 16.71s (± 0.58%) +0.04s (+ 0.25%) 16.53s 17.04s
xstate - node (v10.16.3, x64)
Memory used 569,049k (± 0.02%) 572,637k (± 1.39%) +3,588k (+ 0.63%) 568,875k 604,735k
Parse Time 2.55s (± 0.27%) 2.56s (± 0.37%) +0.01s (+ 0.51%) 2.54s 2.59s
Bind Time 1.01s (± 0.44%) 1.01s (± 0.92%) -0.00s (- 0.40%) 0.99s 1.03s
Check Time 1.50s (± 0.90%) 1.50s (± 0.70%) -0.00s (- 0.27%) 1.48s 1.53s
Emit Time 0.07s (± 0.00%) 0.07s (± 3.14%) +0.00s (+ 1.43%) 0.07s 0.08s
Total Time 5.13s (± 0.30%) 5.14s (± 0.42%) +0.01s (+ 0.14%) 5.09s 5.20s
Angular - node (v12.1.0, x64)
Memory used 332,762k (± 0.03%) 332,601k (± 0.08%) -162k (- 0.05%) 331,565k 332,876k
Parse Time 1.94s (± 0.60%) 1.95s (± 0.66%) +0.01s (+ 0.77%) 1.93s 1.98s
Bind Time 0.82s (± 1.22%) 0.82s (± 0.63%) +0.00s (+ 0.12%) 0.81s 0.83s
Check Time 5.33s (± 0.38%) 5.37s (± 0.66%) +0.04s (+ 0.69%) 5.30s 5.46s
Emit Time 6.15s (± 0.49%) 6.18s (± 0.69%) +0.04s (+ 0.62%) 6.08s 6.27s
Total Time 14.24s (± 0.35%) 14.33s (± 0.44%) +0.09s (+ 0.65%) 14.20s 14.46s
Compiler-Unions - node (v12.1.0, x64)
Memory used 191,583k (± 0.08%) 191,546k (± 0.08%) -38k (- 0.02%) 191,002k 191,747k
Parse Time 0.78s (± 1.04%) 0.78s (± 0.83%) -0.00s (- 0.26%) 0.77s 0.80s
Bind Time 0.53s (± 1.22%) 0.53s (± 0.75%) -0.00s (- 0.19%) 0.52s 0.54s
Check Time 7.34s (± 0.64%) 7.34s (± 0.51%) -0.00s (- 0.03%) 7.26s 7.42s
Emit Time 2.48s (± 1.28%) 2.49s (± 0.82%) +0.01s (+ 0.44%) 2.46s 2.54s
Total Time 11.13s (± 0.54%) 11.14s (± 0.35%) +0.01s (+ 0.08%) 11.06s 11.21s
Monaco - node (v12.1.0, x64)
Memory used 325,599k (± 0.03%) 325,484k (± 0.02%) -115k (- 0.04%) 325,352k 325,651k
Parse Time 1.46s (± 0.59%) 1.46s (± 0.66%) -0.00s (- 0.07%) 1.43s 1.48s
Bind Time 0.73s (± 0.91%) 0.73s (± 0.89%) -0.00s (- 0.27%) 0.72s 0.75s
Check Time 5.39s (± 0.52%) 5.38s (± 0.41%) -0.01s (- 0.26%) 5.32s 5.42s
Emit Time 3.23s (± 0.67%) 3.23s (± 0.68%) -0.01s (- 0.19%) 3.17s 3.28s
Total Time 10.82s (± 0.27%) 10.79s (± 0.38%) -0.03s (- 0.24%) 10.69s 10.87s
TFS - node (v12.1.0, x64)
Memory used 290,244k (± 0.02%) 290,255k (± 0.02%) +10k (+ 0.00%) 290,149k 290,346k
Parse Time 1.22s (± 0.64%) 1.22s (± 0.41%) -0.01s (- 0.49%) 1.21s 1.23s
Bind Time 0.69s (± 0.72%) 0.69s (± 0.58%) -0.01s (- 0.86%) 0.68s 0.70s
Check Time 4.94s (± 0.48%) 4.96s (± 0.30%) +0.01s (+ 0.26%) 4.93s 4.99s
Emit Time 3.40s (± 0.75%) 3.39s (± 0.67%) -0.01s (- 0.35%) 3.35s 3.47s
Total Time 10.26s (± 0.37%) 10.25s (± 0.26%) -0.01s (- 0.09%) 10.19s 10.30s
material-ui - node (v12.1.0, x64)
Memory used 450,130k (± 0.01%) 450,144k (± 0.01%) +14k (+ 0.00%) 450,018k 450,277k
Parse Time 1.78s (± 0.38%) 1.79s (± 0.34%) +0.01s (+ 0.73%) 1.78s 1.81s
Bind Time 0.64s (± 1.09%) 0.64s (± 1.25%) +0.00s (+ 0.16%) 0.63s 0.66s
Check Time 12.71s (± 0.61%) 12.83s (± 0.45%) +0.12s (+ 0.92%) 12.72s 12.96s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.13s (± 0.51%) 15.26s (± 0.37%) +0.13s (+ 0.86%) 15.14s 15.39s
xstate - node (v12.1.0, x64)
Memory used 538,583k (± 1.44%) 538,577k (± 1.44%) -7k (- 0.00%) 534,900k 569,972k
Parse Time 2.48s (± 0.76%) 2.50s (± 0.43%) +0.02s (+ 0.69%) 2.47s 2.52s
Bind Time 1.04s (± 0.66%) 1.05s (± 0.69%) +0.01s (+ 1.16%) 1.03s 1.06s
Check Time 1.43s (± 0.54%) 1.44s (± 0.56%) +0.01s (+ 0.63%) 1.43s 1.46s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.01s (± 0.35%) 5.05s (± 0.30%) +0.04s (+ 0.70%) 5.02s 5.09s
Angular - node (v14.15.1, x64)
Memory used 331,218k (± 0.01%) 331,228k (± 0.01%) +10k (+ 0.00%) 331,137k 331,272k
Parse Time 1.95s (± 1.02%) 1.95s (± 0.73%) +0.00s (+ 0.21%) 1.92s 1.99s
Bind Time 0.86s (± 0.64%) 0.86s (± 0.57%) +0.00s (+ 0.23%) 0.86s 0.88s
Check Time 5.42s (± 0.46%) 5.43s (± 0.74%) +0.01s (+ 0.15%) 5.35s 5.52s
Emit Time 6.21s (± 0.44%) 6.21s (± 0.90%) +0.00s (+ 0.06%) 6.11s 6.36s
Total Time 14.44s (± 0.36%) 14.46s (± 0.58%) +0.02s (+ 0.12%) 14.25s 14.66s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,384k (± 0.55%) 192,488k (± 0.57%) +105k (+ 0.05%) 190,220k 193,491k
Parse Time 0.81s (± 0.72%) 0.81s (± 0.68%) +0.01s (+ 0.62%) 0.80s 0.82s
Bind Time 0.55s (± 0.89%) 0.55s (± 0.73%) -0.00s (- 0.54%) 0.54s 0.56s
Check Time 7.36s (± 0.64%) 7.43s (± 0.53%) +0.08s (+ 1.07%) 7.33s 7.50s
Emit Time 2.48s (± 0.70%) 2.48s (± 0.59%) -0.00s (- 0.04%) 2.43s 2.51s
Total Time 11.19s (± 0.53%) 11.27s (± 0.39%) +0.08s (+ 0.72%) 11.16s 11.39s
Monaco - node (v14.15.1, x64)
Memory used 324,371k (± 0.00%) 324,363k (± 0.01%) -8k (- 0.00%) 324,331k 324,417k
Parse Time 1.51s (± 0.62%) 1.51s (± 0.54%) -0.00s (- 0.07%) 1.49s 1.52s
Bind Time 0.76s (± 0.65%) 0.76s (± 0.85%) +0.00s (+ 0.26%) 0.74s 0.77s
Check Time 5.33s (± 0.50%) 5.37s (± 0.47%) +0.04s (+ 0.79%) 5.30s 5.41s
Emit Time 3.28s (± 1.28%) 3.27s (± 0.89%) -0.00s (- 0.03%) 3.24s 3.36s
Total Time 10.87s (± 0.61%) 10.91s (± 0.40%) +0.04s (+ 0.39%) 10.82s 11.01s
TFS - node (v14.15.1, x64)
Memory used 289,164k (± 0.01%) 289,190k (± 0.00%) +26k (+ 0.01%) 289,146k 289,222k
Parse Time 1.23s (± 0.45%) 1.23s (± 0.30%) -0.00s (- 0.24%) 1.22s 1.23s
Bind Time 0.73s (± 0.88%) 0.73s (± 1.11%) +0.00s (+ 0.14%) 0.72s 0.76s
Check Time 4.95s (± 0.41%) 4.95s (± 0.65%) 0.00s ( 0.00%) 4.88s 5.04s
Emit Time 3.50s (± 1.57%) 3.50s (± 0.74%) +0.00s (+ 0.09%) 3.46s 3.58s
Total Time 10.41s (± 0.61%) 10.41s (± 0.45%) +0.01s (+ 0.07%) 10.34s 10.51s
material-ui - node (v14.15.1, x64)
Memory used 448,338k (± 0.01%) 448,331k (± 0.00%) -7k (- 0.00%) 448,290k 448,364k
Parse Time 1.83s (± 0.47%) 1.83s (± 0.61%) +0.00s (+ 0.11%) 1.81s 1.87s
Bind Time 0.68s (± 0.66%) 0.68s (± 0.69%) +0.00s (+ 0.29%) 0.67s 0.69s
Check Time 12.81s (± 0.52%) 12.96s (± 0.94%) +0.15s (+ 1.17%) 12.78s 13.39s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.33s (± 0.40%) 15.48s (± 0.82%) +0.15s (+ 1.00%) 15.27s 15.91s
xstate - node (v14.15.1, x64)
Memory used 532,818k (± 0.01%) 532,834k (± 0.01%) +16k (+ 0.00%) 532,755k 532,908k
Parse Time 2.54s (± 0.32%) 2.55s (± 0.53%) +0.01s (+ 0.55%) 2.53s 2.58s
Bind Time 1.15s (± 1.17%) 1.15s (± 0.77%) +0.01s (+ 0.52%) 1.14s 1.18s
Check Time 1.48s (± 0.32%) 1.49s (± 0.58%) +0.01s (+ 0.74%) 1.47s 1.51s
Emit Time 0.07s (± 0.00%) 0.07s (± 3.14%) +0.00s (+ 1.43%) 0.07s 0.08s
Total Time 5.24s (± 0.28%) 5.27s (± 0.40%) +0.02s (+ 0.48%) 5.23s 5.31s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory9 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 47049 10
Baseline main 10

Developer Information:

Download Benchmark

weswigham
weswigham reviewed Dec 7, 2021
View reviewed changes
src/compiler/binder.ts Outdated
if (!state.skip) {
const operator = node.operatorToken.kind;

if (operator === SyntaxKind.CommaToken) {
Copy link
Member

@weswigham weswigham Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be in onExit and not onRight?

Copy link
Member Author

@jakebailey jakebailey Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look up a bit, the node.left is processed in onOperator, presumably because there's some work that is completed after onLeft is completed. That's how it was added in #44487.

I tried putting it into onRight first and hit infinite recursion, then realized that the other call wasn't in onLeft and moved it down.

Copy link
Member Author

@jakebailey jakebailey Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I can get away with moving these around, though, and try running them after maybeBind in onLeft and onRight instead. I'll try it. It'd sure look less weird.

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Member Author

@jakebailey jakebailey Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved things around to be a little less surprising.

@jakebailey jakebailey merged commit e2c0033 into microsoft:main Jan 18, 2022
@jakebailey jakebailey deleted the fix-41312 branch January 18, 2022 19:46
@Andarist Andarist mentioned this pull request Feb 3, 2025
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham weswigham weswigham approved these changes

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

@jakebailey jakebailey

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comma Operator breaks Assertion Functions

3 participants

@jakebailey @typescript-bot @weswigham